-
-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improv: add custom modifier example #88
improv: add custom modifier example #88
Conversation
991d9e6
to
df03641
Compare
lib/src/parse/parse.dart
Outdated
@@ -431,32 +427,15 @@ void parse<D>( | |||
|
|||
if (elementSpec.modifiers != null) { | |||
for (var modifier in elementSpec.modifiers!) { | |||
GeomModifierOp geomModifier; | |||
if (modifier is DodgeModifier) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most important change: we no longer need to check the instance type of modifier
. Every modifier must implement a toGeomModifierOp
that returns the corresponding GeomModifierOp
, which allows everyone to specify their custom modifiers.
lib/src/geom/modifier/modifier.dart
Outdated
/// The only use of this class to pass parameters to [Modifier.toGeomModifierOp]. | ||
/// A class is used, instead of named arguments, to avoid breaking changes when | ||
/// adding new fields to these parameters. | ||
class ToGeomModifierOpParams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to use a class here so we can add new parameters in the future without creating a breaking changes for consumers of this API.
Unfortunately Dart's named arguments require consumers to specify all parameters, if ones that aren't used, so adding a new argument to named arguments would be a breaking change.
lib/src/dataflow/tuple.dart
Outdated
@@ -66,7 +66,7 @@ class Aes { | |||
final Label? label; | |||
|
|||
/// The size of the tuple. | |||
final double? size; | |||
double? size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to remove the final
keyword from this property so that it can be changed from a modifier.
This is similar to ggplot
's dodge2
modifier that also changes the size of its elements
ce76632
to
61bd9af
Compare
Hey, @entronad! This changes core logic in Thank you! |
I would rather not merge this PR after a long time of thinking. Reasons are:
But I really get some inspiration from this PR, thanks. I think what you need (to auto adjust the bar size) can be implemented by a custom Shape, that is what recommended by Graphic. maybe that's not as simple as to change the modifier, but I think it worth a try. |
Hey, @entronad! I understand your concerns. On top of changing the width of the interval element, we have another use case where we need to stack interval elements, but the logic differs from the In the In that use case, using a custom shape does not seem like a possible solution, right? What can we do there? That would be another use case for custom modifiers.
I understand your concern. Maybe we can figure out a better API for this? Instead of having 3 different classes, we can probably have just one
Do you have any ideas in mind regarding how this might be implemented? As you probably have noticed, I'd like to contribute and make graphic a better library, so if you have any suggestion, I could try implementing it, if it meant that we would have this done a bit faster 😄 Thanks! |
Your cases resolved my doubt. I am convinced to add custom modifier now.
I will think it over and make the commits. Maybe this PR will be used, but thank you all the same, tips from this PR are very usefull. |
Good to know, @entronad! 😄 |
Added in v0.10.0 |
Thank you, @entronad 🙏 Do you think it would make sense to update this PR to add the custom modifier example? Or should I close this PR? |
Also, @entronad, it seems like |
Added these params types in v0.10.1 You are welcome to add these custom modifier examples, thank you very much. You can either edit this PR or post a new one. |
61bd9af
to
c9406f2
Compare
lib/graphic.dart
Outdated
export 'src/scale/discrete.dart' show DiscreteScale, DiscreteScaleConv; | ||
export 'src/scale/continuous.dart' show ContinuousScale, ContinuousScaleConv; | ||
export 'src/scale/linear.dart' show LinearScale, LinearScaleConv; | ||
export 'src/scale/ordinal.dart' show OrdinalScale, OrdinalScaleConv; | ||
export 'src/scale/time.dart' show TimeScale, TimeScaleConv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add these exports for the modifier to work
final numGroups = aesGroups.length; | ||
final groupHorizontalPadding = _kBaseGroupPaddingHorizontal / numGroups; | ||
final invertedGroupPaddingHorizontal = | ||
coord.invertDistance(groupHorizontalPadding, _kXAxis); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@entronad I need to get this coord
, but I can't seem to get it with the current implementation. Do you have any idea how I can convert pixels into the [0, 1] range?
I will add all these. |
I have add the coord in a new commit, please have a try. |
c9406f2
to
accff22
Compare
accff22
to
ee7c943
Compare
@@ -18,7 +18,7 @@ import 'package:graphic/src/shape/shape.dart'; | |||
/// Rendering customization should be in the [Shape]. | |||
abstract class Modifier extends CustomizableSpec { | |||
/// Modifies the position of element items. | |||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result of running flutter format .
, which was failing the "ci" GitHub Action
It should be ready, @entronad 😄 |
Can we create a new release with the new exports? I'd like to use this modifier for another project 😄 |
Published in v0.10.2 |
This PR adds an example of a custom
Modifier
. It creates aDodgeSizeModifier
that changes an element's position and width depending on the number of elements in a band.